fix: use shared uv cache for package tests#832
Conversation
📝 WalkthroughWalkthroughThree targeted changes to Changespkg-test Makefile updates
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
python/Makefile (2)
102-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare
pkg-test-allas a PHONY target.The
pkg-test-alltarget doesn't create a file with that name, so it should be declared as.PHONYto ensure it always runs even if a file namedpkg-test-allexists in the directory.📝 Proposed fix to add PHONY declaration
Add the target to the
.PHONYdeclaration (likely near the top or bottom of the Makefile):+.PHONY: pkg-test-all🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/Makefile` at line 102, The target `pkg-test-all` does not have a PHONY declaration, which means Make will treat it as a file target and may skip execution if a file with that name exists in the directory. Add `pkg-test-all` to the existing `.PHONY` declaration at the top of the Makefile (typically near other PHONY declarations like `.PHONY: sync, test, etc.`) to ensure this target always executes regardless of whether a file with that name is present.Source: Linters/SAST tools
88-88: 📐 Maintainability & Code Quality | 🔵 TrivialConsider adding
PYTHONUNBUFFERED=1to the non-LOGS_DIR path for consistency.Currently,
PYTHONUNBUFFERED=1is set only on line 88 (where output is piped throughteeto a log file), but not on line 96 (direct terminal output). While unbuffered output is essential for real-time log capture withtee, adding it to line 96 would provide immediate test output feedback during local development without needing to wait for buffer flushes. This is optional but improves consistency across both code paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/Makefile` at line 88, The `PYTHONUNBUFFERED=1` environment variable is currently only set on the pytest command that pipes output to a log file via tee (line 88), but is missing from the other pytest invocation that outputs directly to the terminal (line 96). Add `PYTHONUNBUFFERED=1` to the `uv run` command on line 96 to match the configuration on line 88 and ensure consistent unbuffered output behavior across both code paths, providing immediate test feedback during local development.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@python/Makefile`:
- Line 102: The target `pkg-test-all` does not have a PHONY declaration, which
means Make will treat it as a file target and may skip execution if a file with
that name exists in the directory. Add `pkg-test-all` to the existing `.PHONY`
declaration at the top of the Makefile (typically near other PHONY declarations
like `.PHONY: sync, test, etc.`) to ensure this target always executes
regardless of whether a file with that name is present.
- Line 88: The `PYTHONUNBUFFERED=1` environment variable is currently only set
on the pytest command that pipes output to a log file via tee (line 88), but is
missing from the other pytest invocation that outputs directly to the terminal
(line 96). Add `PYTHONUNBUFFERED=1` to the `uv run` command on line 96 to match
the configuration on line 88 and ensure consistent unbuffered output behavior
across both code paths, providing immediate test feedback during local
development.
Remove per-package UV_CACHE_DIR override from Makefile test targets. Previously each package set UV_CACHE_DIR to a separate directory (.uv-cache/<pkg>), preventing packages from sharing cached downloads and built wheels. This caused redundant downloads and builds of common dependencies (grpcio, protobuf, etc.) across every package test run. By using uv's default shared cache (~/.cache/uv on Linux, ~/Library/Caches/uv on macOS), all package test venvs now reuse the same cached artifacts, significantly speeding up test runs.
Run 'uv sync --all-packages --all-extras' before parallel pkg-test-% targets to ensure all dependencies are already cached. This avoids cache write contention during parallel test runs while still benefiting from a shared cache.
mangelajo
force-pushed
the
fix/shared-uv-cache
branch
from
6824831 to
46a8f9b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/Makefile`:
- Line 102: The `pkg-test-all` target is not declared as a phony target in the
Makefile, which can cause Make to skip execution if a file or directory named
`pkg-test-all` exists. Add `pkg-test-all` to the `.PHONY` declaration at the top
of the Makefile alongside other phony targets to ensure this target always
executes regardless of filesystem state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| uv run --isolated --directory $< ty check . | ||
|
|
||
| pkg-test-all: $(addprefix pkg-test-,$(PKG_TARGETS)) | ||
| pkg-test-all: sync $(addprefix pkg-test-,$(PKG_TARGETS)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Declare pkg-test-all as phony to avoid accidental no-op builds.
If a file/directory named pkg-test-all exists, make can treat the target as up-to-date and skip running the package tests.
Suggested fix
+.PHONY: pkg-test-all
pkg-test-all: sync $(addprefix pkg-test-,$(PKG_TARGETS))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pkg-test-all: sync $(addprefix pkg-test-,$(PKG_TARGETS)) | |
| .PHONY: pkg-test-all | |
| pkg-test-all: sync $(addprefix pkg-test-,$(PKG_TARGETS)) |
🧰 Tools
🪛 checkmake (0.3.2)
[warning] 102-102: Target "pkg-test-all" should be declared PHONY.
(phonydeclared)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/Makefile` at line 102, The `pkg-test-all` target is not declared as a
phony target in the Makefile, which can cause Make to skip execution if a file
or directory named `pkg-test-all` exists. Add `pkg-test-all` to the `.PHONY`
declaration at the top of the Makefile alongside other phony targets to ensure
this target always executes regardless of filesystem state.
Source: Linters/SAST tools
2 participants
Problem
The Makefile test targets (
pkg-test-%) setUV_CACHE_DIRto a separate directory per package (.uv-cache/<pkg>). This means each package's test venv downloads and builds its own copy of shared dependencies likegrpcio,protobuf, etc., even though they're identical across packages.Fix
Remove the
UV_CACHE_DIRoverride entirely, lettinguvuse its default shared cache (~/.cache/uvon Linux,~/Library/Caches/uvon macOS). This way all package test runs share cached downloads and built wheels, avoiding redundant work.Impact
UV_CACHE_DIRvia environment if a custom cache path is needed for persistence